[WIP] Derive property groups #855

parasyte · 2022-02-06T10:48:47Z

Adds support for property categories and groups:
https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_exports.html#adding-script-categories
https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_exports.html#grouping-properties

This was thrown together to start a discussion. There are no tests, and I am not happy with the implementation. Consider this PR an experiment with no intention to merge. But it could help define the feature and identify shortcomings.

Usage currently looks something like this:

use gdnative::{
    // GroupMarker is a special unit struct to deal with the way Godot property groups
    // are implemented in property lists.
    export::GroupMarker,
    prelude::*,
};

#[derive(NativeClass)]
#[inherit(Node)]
pub struct MyStruct {
    /// This property is _not_ grouped.
    #[property(default = 8.0, after_set = "Self::update_callback")]
    magnitude: f32,

    /// This property defines a _name_ for your group.
    #[property(group)]
    offset: GroupMarker,

    /// This property directly follows a GroupMarker, so it is a member of the group.
    /// The property name in the editor will be "Offset X".
    #[property(after_set = "Self::update_callback"]
    offset_x: f32,

    /// This property will also be a member of the previously defined group.
    /// The property name in the editor will be "Offset Y".
    #[property(after_set = "Self::update_callback"]
    offset_y: f32,

    /// This property defines a new group and sets a hint prefix for the editor.
    /// The prefix is used by the editor to match property names.
    /// The prefix is removed from the member names.
    #[property(group = "size_")]
    size: GroupMarker,

    /// This property has a prefix matching the previous GroupMarker, so it is a member of the group.
    /// The property name in the editor will be "Width".
    #[property(default = 5, after_set = "Self::update_callback"]
    size_width: u32,

    /// This property has a prefix matching the previous GroupMarker, so it is a member of the group.
    /// The property name in the editor will be "Height".
    #[property(default = 5, after_set = "Self::update_callback"]
    size_height: u32,

    /// This property _does not_ have a prefix matching the previous GroupMarker,
    /// so it is _not_ a member of the group.
    /// The property will appear in the editor below "Magnitude".
    /// This is the behavior of Godot.
    #[property(after_set = "Self::update_callback"]
    duration: f32,
}

How it looks in the editor:

Ideally, I believe that property groups should be declared structurally. The example above has a flat hierarchy that illustrates many issues:

The code is generally not easy to read. It is difficult to tell at a glance which properties are grouped together.
The editor hint provided by the group = "..." arg is easy to typo, which will ruin the grouping of items. Likewise, property names are only conventionally grouped by a prefix.
Godot's behavior with property groups leaves a lot to be desired:
- Defining a group requires a property whose value is Nil which provides a name for the group. In the example, size and offset are both group names, created automatically by the property attribute using the field name as the property's name. The GroupMarker type is just a way to feed the Nil value to Godot.
- All properties following another property with the GROUP usage flag enabled will become a member of that group. (See the linked GDScript documentation.)
- The behavior changes when a hint_string is provided on the property with the GROUP flag: In this case, only properties whose name matches the given hint prefix will become group members. Any other properties will "move" to the top of the list outside of any group. This behavior is surprising, and any number of bugs can cause this to happen by mistake.

With structural declaration, the example could possibly look like this:

use gdnative::prelude::*;

#[derive(PropertyGroup)]
struct Offset {
    /// Offset along X-axis in meters.
    #[property(after_set = "MyStruct::update_callback"]
    x: f32,

    /// Offset along Y-axis in meters.
    #[property(after_set = "MyStruct::update_callback"]
    y: f32,
}

#[derive(PropertyGroup)]
struct Size {
    /// Width in meters.
    #[property(default = 5, after_set = "MyStruct::update_callback"]
    width: u32,

    /// Height in meters.
    #[property(default = 5, after_set = "MyStruct::update_callback"]
    height: u32,
}

#[derive(NativeClass)]
#[inherit(Node)]
pub struct MyStruct {
    /// This property is _not_ grouped.
    #[property(default = 8.0, after_set = "Self::update_callback")]
    magnitude: f32,

    /// This property defines a _name_ for your group and all of its members.
    #[property(group, no_group_prefix)]
    offset: Offset,

    /// Define another group with its members.
    #[property(group)]
    size: Size,

    /// This property does not belong to a group.
    /// So it will appear in the editor below "Magnitude".
    /// This is the behavior of Godot.
    #[property(after_set = "Self::update_callback"]
    duration: f32,
}

This looks more reasonable, but it also has some problems:

Godot properties must have a unique name. The derive macro will need to flatten the struct into a single-level property list with e.g. size.width being renamed to size_width and a hint on the group set to "size_" to handle the prefixing. Creating a group without prefix handling would have to be an extra option for the property attribute.
The flattening means that getting a property by name may be surprising behavior! Users will have to know that the group creates magically prefixed property names if they ever need to look them up with Godot's property list functions.
Hypothetically this could support arbitrary nesting depths, but for simplicity I personally prefer a maximum depth of 2.
The way Godot "groups" ungrouped properties at the top of the list is a bit unexpected. But I guess they had to make the UI reasonable. There's nothing we can do about that.
Lacking unnamed structs in Rust is unfortunate. We need to declare Offset and Size as separate types, and they both have to refer back to MyStruct for getter/setter functions. At least it makes sense to include hint functions on these separate types.
Honestly, I don't know how well this structural property declaration will work. Do we still need #[property(group)] for this? Is ``#[derive(PropertyGroup)]` enough? Can we do something better?

- Adds support for property categories and groups: https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_exports.html#adding-script-categories https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_exports.html#grouping-properties

Bromeon · 2022-04-25T21:45:53Z

For Rust, we should probably follow a slightly different approach than in GDScript. I agree that the behavior is suprising and has room for improvement.

Some ideas what we could do better in Rust:

Type safety + no magic
String-based grouping can work in a small scope (and be actually simpler than something type-safe), but it's also easy to type-safe at compile time.
The "prefix defines group" is also something very unusual in Rust, I'd rather have things explicit.
Stateless
The "previous declaration decides group" behavior is horrific and introduces arbitrary restrictions to Rust, such as not being able to reorder fields. Each field should determine its group affinity in a self-contained way.
One way to do things
I'd claim this is a relatively rarely used feature, so the API surface for it should be small. If the user wants to achieve a certain layout of the UI elements, there should ideally be one way to achieve this in Rust. That also helps recognizability across code.

I'm not yet sure about grouping via struct or via label (attribute), it depends a bit how this would be used. Struct would be the "more correct" way, but keep in mind that this means that the export layout determines how Rust code needs to access the fields. Which can be a good thing (when improving code structure), or super annoying (when forcing introduction of 5 new types for seemingly no benefit). I'd still say we should choose one way here.

In case we go for the structural approach, your 2nd proposed API already looks quite nice.

In a label-based world, I would probably change some things:

// Group is named "Offset", no prefix for fields
#[derive(PropertyGroup)]
struct Offset;

// Group is named "Size", fields prefixed "size_"
#[derive(PropertyGroup)]
#[group(title = "Size", prefix = "size_")]
struct SizeGroup;

#[derive(NativeClass)]
#[inherit(Node)]
pub struct MyStruct {
    // This property is _not_ grouped.
    // Exported as "magnitude"
    #[property(default = 8.0, set = "Self::set_magnitude")]
    magnitude: f32,

    // Exported as "offset_x"
    #[property(group = Offset)]
    offset_x: f32,

    // Exported as "offset_y"
    #[property(group = Offset)]
    offset_y: f32,

    // Exported as "size_width"
    #[property(group = SizeGroup)]
    width: u32,

    // Exported as "size_height"
    #[property(group = SizeGroup)]
    height: u32,

    // Not member of a group simply because there's no `group` attribute
    // No need to inspect any field declarations before
    #[property]
    duration: f32,
}

Not fully happy with the prefix magic though. We might as well use an explicit #[property(group = SizeGroup, rename = "size_width")] syntax even if it means some repetition...

Bromeon · 2022-06-05T19:04:40Z

@parasyte Any feedback on my ideas above? 💡

chitoyuu · 2022-12-02T12:14:28Z

It seems like this PR has fallen into inactivity for a while, and has now drifted too far from master to be merged as is. @parasyte Please let us know if you're still interested in the idea. Otherwise, I'll be closing this in a few days.

Note that property grouping via renaming has been available since at least 0.9.2: #[property(path = "my_category/my_property_name")]. See https://godot-rust.github.io/docs/gdnative/derive/derive.NativeClass.html#property. Are there any mechanical differences between properties grouped this way and ones grouped via PropertyUsage::GROUP? This could help us determine if we should consider this as a missing feature, or purely a QoL improvement.

parasyte · 2022-12-02T18:11:50Z

Closing this is the right thing to do. I can’t comment on how it compares to the #[property(path = "…")] attribute. But as long as it supports grouping the same way as shown in the screenshot then this PR has nothing new to offer. I’ll leave it to you to make that evaluation.

Thanks for the reminder on this.

parasyte force-pushed the feature/derive-property-groups branch from c53620a to 234a37d Compare February 6, 2022 11:19

Bromeon added c: export Component: export (mod export, derive) feature Adds functionality to the library labels Feb 6, 2022

Bromeon added this to the v0.10.1 milestone Feb 6, 2022

parasyte force-pushed the feature/derive-property-groups branch from 234a37d to 1828e4d Compare February 6, 2022 17:03

parasyte force-pushed the feature/derive-property-groups branch from 1828e4d to 6832f39 Compare February 14, 2022 19:07

Bromeon modified the milestones: v0.10.1, v0.10.2 Jul 16, 2022

Bromeon mentioned this pull request Aug 25, 2022

[Question] Access properties of a property through the Godot Editor #932

Closed

Bromeon modified the milestones: v0.10.2, v0.11.x Oct 1, 2022

Bromeon added the status: postponed label Oct 18, 2022

Bromeon modified the milestones: v0.11.x, unplanned Oct 18, 2022

parasyte closed this Dec 2, 2022

parasyte deleted the feature/derive-property-groups branch January 9, 2023 03:43

Bromeon mentioned this pull request Apr 6, 2023

Add #[export_group] Attribute for Creating Property Groups godot-rust/gdext#214

Closed

gg-yb mentioned this pull request Apr 12, 2023

Grouping Exported Properties in the Editor godot-rust/gdext#226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Derive property groups #855

[WIP] Derive property groups #855

Uh oh!

parasyte commented Feb 6, 2022 •

edited

Loading

Uh oh!

Bromeon commented Apr 25, 2022

Uh oh!

Bromeon commented Jun 5, 2022

Uh oh!

chitoyuu commented Dec 2, 2022 •

edited

Loading

Uh oh!

parasyte commented Dec 2, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[WIP] Derive property groups #855

[WIP] Derive property groups #855

Uh oh!

Conversation

parasyte commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Apr 25, 2022

Uh oh!

Bromeon commented Jun 5, 2022

Uh oh!

chitoyuu commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parasyte commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

parasyte commented Feb 6, 2022 •

edited

Loading

chitoyuu commented Dec 2, 2022 •

edited

Loading

parasyte commented Dec 2, 2022 •

edited

Loading